Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Looks up for kid and alg also in unprotected header #25

Merged
merged 6 commits into from
Nov 27, 2021

Conversation

lmammino
Copy link
Member

@lmammino lmammino commented Nov 26, 2021

Fixes #1

  • Parses kid and alg from unprotected header.
  • Uses SHA256 hash to calculate kid from certificates
  • Removes header_unprotected (not used anymore) and makes header_protected_raw and payload_raw private in Cwt (it felt like a leaking abstraction)
  • Makes sure all the tests originally skipped because of Support kid in unprotected header #1 now run correctly

Note: It is better to merge #23 first and then rebase this branch, so we can easily test against all the previously failing tests. Done

CC: @rez23 who was already looking at this!

@lmammino lmammino self-assigned this Nov 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #25 (672e19f) into main (ff6ae22) will increase coverage by 0.02%.
The diff coverage is 82.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   82.34%   82.36%   +0.02%     
==========================================
  Files          10       10              
  Lines        1048     1072      +24     
==========================================
+ Hits          863      883      +20     
- Misses        185      189       +4     
Impacted Files Coverage Δ
src/trustlist.rs 58.87% <75.00%> (+0.67%) ⬆️
src/cwt.rs 79.41% <80.70%> (-1.10%) ⬇️
src/parse.rs 73.91% <87.50%> (+0.72%) ⬆️
src/dgc_cert.rs 97.08% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6ae22...672e19f. Read the comment docs.

@lmammino lmammino requested a review from rez23 November 26, 2021 11:04
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may squash the two commits in one while at it.

src/cwt.rs Outdated Show resolved Hide resolved
@lmammino lmammino force-pushed the support-kid-in-unprotected-header branch from b4531ae to e268f09 Compare November 26, 2021 18:01
@lmammino lmammino requested a review from lu-zero November 26, 2021 19:51
src/dgc_cert.rs Outdated
Comment on lines 16 to 24
fn t_empty_if_null<'de, D>(deserializer: D) -> Result<Vec<Test>, D::Error>
where
D: Deserializer<'de>,
{
let opt = Option::deserialize(deserializer)?;
Ok(opt.unwrap_or_default())
}

fn v_empty_if_null<'de, D>(deserializer: D) -> Result<Vec<Vaccination>, D::Error>
where
D: Deserializer<'de>,
{
let opt = Option::deserialize(deserializer)?;
Ok(opt.unwrap_or_default())
}

fn r_empty_if_null<'de, D>(deserializer: D) -> Result<Vec<Recovery>, D::Error>
where
D: Deserializer<'de>,
{
let opt = Option::deserialize(deserializer)?;
Ok(opt.unwrap_or_default())
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I had to do this because there were 2 tests (originally skipped because of the "kid in unprotected header" problem) that have the following content encoded:

 {
    "r": null,
    "t": null,
    "v":  [
       // ...
    ],
}

Instead of having only one value set between r, t, or v they set two of them to null and the other to an appropriate array.

Solution based on serde-rs/serde#1098

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have generalised this to one generic empty_if_null function...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: https://docs.rs/serde_with/1.11.0/serde_with/
This can be pretty useful...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one. Is there any utility there that would do what we are doing here? I couldn't find one at first glance...

Copy link
Contributor

@dodomorandi dodomorandi Nov 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I am not sure. However, this a lifesaver to avoid skip_serialization_if(Option::is_none) everywhere. For some of my projects I discovered this a bit too late... 😓

@lmammino
Copy link
Member Author

@lu-zero, this is now rebases with main. I can do a "Squash and merge" once this is ready to go rather than squashing everything upfront. Would that work for you?

@lmammino
Copy link
Member Author

Note that with this PR we solve most of the broken test. We go from:

test result: ok. 528 passed; 0 failed; 52 ignored; 0 measured; 0 filtered out; finished in 0.17s

To:

test result: ok. 558 passed; 0 failed; 22 ignored; 0 measured; 0 filtered out; finished in 0.17s

So 30 new tests are now passing! 🎉

Copy link
Contributor

@dodomorandi dodomorandi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to propose a commit (it would be a little bit a mess using diff changes), feel free to reject it if you find it out of scope. I noticed that some parts have been refactored but they could be improved a little bit.

@dodomorandi
Copy link
Contributor

This is the change I was talking about. Let me know what do you think about it.

Copy link

@guerinoni guerinoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@lmammino
Copy link
Member Author

I love the suggested changes @dodomorandi! Thanks.

I actually contributed to ciborium adding the existing as_x functions to Value. It would be nice to submit a PR with these new features. You should do it 😉

I'll try to fix the conflict, rebase and get this merged today 🤞

@dodomorandi
Copy link
Contributor

I'll try to fix the conflict, rebase and get this merged today 🤞

No worries, I'll do it because I caused the conflict 😁. I was just waiting for your feedback before performing a forced push.

It would be nice to submit a PR with these new features. You should do it 😉

I totally agree!!

lmammino and others added 6 commits November 27, 2021 10:43
Unfortunately `ciborium` misses some ergonomic features in order to keep
things easy. I added an _extension trait_ to overcome this problem and I
used it to keep code a little bit cleaner.
@dodomorandi dodomorandi force-pushed the support-kid-in-unprotected-header branch from b4e22f3 to 672e19f Compare November 27, 2021 10:49
@dodomorandi dodomorandi merged commit 5e4db0f into main Nov 27, 2021
@dodomorandi dodomorandi deleted the support-kid-in-unprotected-header branch December 9, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support kid in unprotected header
5 participants